Upgrade bundling #2187

dy · 2017-11-28T23:06:27Z

introduces minify-stream instead of uglifyjs2 - less errors, better options
removes patch_minified (seems that uglify-es fixes that since tests pass)
introduces bubleify to handle ES6 dependencies

Conflicts: package.json

etpinard · 2017-11-28T23:36:55Z

tasks/util/constants.js

+    },
+
+    bubleifyOptions: {
+        target: {


Hmm. We have to target browsers waaaaaaaay older than that. Can bubleify guarantee to output code that IE11 understands?

@etpinard that is the best bubleify can do, it is targeted only for ES6 → ES5, not ES5 → ES3.

Hmm. Well that won't work then. plotly.js needs to be pure ES5.

@etpinard that is pure ES5, same as what we have now. target flag only indicates what new ES6 features are supported by target environment to avoid their handling, eg. some browsers have Object.assign or arrow functions, but do not do power a ** b etc, so since we indicate oldest target available in buble, we make sure we get pure ES5.

Could we put these config options in the package.json instead, so that all consumers bundling with browserify or webpack+ify-loader get the same results as us in our dist/?

Can the output bundle be minified by plain-old uglify-js? e.g. does

browserify -t bubleify src/plotly.js | uglify-js > bundle.js

work?

I'm not a big fan of this target option. I find it pretty confusing. Could we instead manually list all the transforms we want turned on?

etpinard · 2017-11-28T23:38:37Z

@dfcreative can you run npm run bundle && git diff -- dist/README.md and paste a screenshot of the diff to get an idea of how the new minifiers perform?

dy · 2017-11-28T23:52:48Z

How serious is that? Should I try to squeeze that more?

etpinard

How serious is that? Should I try to squeeze that more?

I think so. That's an awful lot extra bytes. I see that minify-stream is using uglify-es. Is there a way to make it more agressively minify things? Or maybe we should just use plain-old uglify-js

Oh well, after thinking about this some more, transpiling our deps down to ES5 is inevitable if we want to stay up-to-date will latest development. So thanks for getting the ball rolling @dfcreative

etpinard · 2017-11-29T15:04:00Z

tasks/util/constants.js

+    },
+
+    bubleifyOptions: {
+        target: {


Could we put these config options in the package.json instead, so that all consumers bundling with browserify or webpack+ify-loader get the same results as us in our dist/?

etpinard · 2017-11-29T15:05:23Z

tasks/util/constants.js

+    },
+
+    bubleifyOptions: {
+        target: {


Can the output bundle be minified by plain-old uglify-js? e.g. does

browserify -t bubleify src/plotly.js | uglify-js > bundle.js

work?

etpinard · 2017-11-29T15:06:39Z

tasks/util/constants.js

+    },
+
+    bubleifyOptions: {
+        target: {


I'm not a big fan of this target option. I find it pretty confusing. Could we instead manually list all the transforms we want turned on?

etpinard · 2017-11-29T15:06:47Z

tasks/util/patch_minified.js

- *
- */
-module.exports = function patchMinified(minifiedCode) {
-    return minifiedCode.replace(PATTERN, NEW_SUBSTR);


One of the hackiest thing in plotly.js at the moment. Good riddance 🎉

etpinard · 2017-11-29T15:06:57Z

tasks/stats.js

@@ -1,6 +1,6 @@
 var path = require('path');
 var fs = require('fs');
-var spawn = require('child_process').spawn;
+var spawn = require('cross-spawn');


Ha nice catch. That's for that spawn('npm', ['ls', '--json', '--only', 'prod']); statement below - which will be obsolete when we switch to npm@5 as the new package-lock.json file will do that same as the current dist/npm-ls.json.

dy · 2017-11-29T21:58:50Z

Actually that diff is not that scary, the actual master has the same size:

It is just the dist/README.md we have is old.

etpinard · 2017-11-29T22:03:08Z

It is just the dist/README.md we have is old.

Ha. I see. Yeah we have a bunch of new things in master that didn't get released yet. 👌

etpinard · 2017-11-30T15:32:23Z

For reference:

on the latest master:

off this branch:

Lovely ❤️

etpinard · 2017-11-30T15:34:54Z

Great PR. Thanks for doing this @dfcreative

Getting rid of patch_minified.js well worth swapping uglify-js for uglify-es.

Merge away 💃

ndabAP · 2018-01-12T15:22:17Z

My Webpack code splitted asset increased by approximately 200 kilobytes after updating to 1.32.0.

etpinard · 2018-01-12T15:28:23Z

That's because we added a lot of features (sorry 😉 ).

But still 200 kB seems larger than the worse case increase (see bundle size changes from 1.31.2 to 1.32.0 here). Perhaps something is off. Would you care to share a reproducible example of your bundle setup and config?

ndabAP · 2018-01-12T15:33:25Z

That's because we added a lot of features

Got it. Would still be nice to be more selective about the features. It would be cool to have the possibility to omit special features like chart zooming, saving as picture or maybe animations to get a smaller package.

Would you care to share a reproducible example of your bundle setup and config?

Unfortunately, my asset has a lot of Vue.js related stuff in it. It would take weeks to sort it out ;) It's not exactly 200 kilobytes, though.

etpinard · 2018-01-12T15:41:36Z

Would still be nice to be more selective about the features.

Yep, we know. That'll have to wait for v2 though.

dy added 2 commits November 28, 2017 18:02

Upgrade bundling to uglify-es

328af5f

Conflicts: package.json

Remove unused dep

41be06f

dy requested a review from etpinard November 28, 2017 23:08

Lintify

a399671

etpinard reviewed Nov 28, 2017

View reviewed changes

Use cross-spawn for windows stats calc

a053c26

etpinard reviewed Nov 29, 2017

View reviewed changes

Remove bubleify

25fd2e4

etpinard added status: reviewable labels Dec 1, 2017

etpinard merged commit 579d3cc into master Dec 1, 2017

etpinard deleted the bundle-up branch December 1, 2017 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade bundling #2187

Upgrade bundling #2187

dy commented Nov 28, 2017 •

edited

Loading

etpinard Nov 28, 2017

dy Nov 28, 2017 •

edited

Loading

etpinard Nov 28, 2017

dy Nov 29, 2017 •

edited

Loading

etpinard Nov 29, 2017

etpinard Nov 29, 2017

etpinard Nov 29, 2017

etpinard commented Nov 28, 2017

dy commented Nov 28, 2017 •

edited

Loading

etpinard left a comment

etpinard Nov 29, 2017

etpinard Nov 29, 2017

etpinard Nov 29, 2017

etpinard Nov 29, 2017

etpinard Nov 29, 2017

dy commented Nov 29, 2017

etpinard commented Nov 29, 2017

etpinard commented Nov 30, 2017

etpinard commented Nov 30, 2017

ndabAP commented Jan 12, 2018 •

edited

Loading

etpinard commented Jan 12, 2018

ndabAP commented Jan 12, 2018

etpinard commented Jan 12, 2018

Upgrade bundling #2187

Upgrade bundling #2187

Conversation

dy commented Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

dy Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dy Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Nov 28, 2017

dy commented Nov 28, 2017 • edited Loading

etpinard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dy commented Nov 29, 2017

etpinard commented Nov 29, 2017

etpinard commented Nov 30, 2017

etpinard commented Nov 30, 2017

ndabAP commented Jan 12, 2018 • edited Loading

etpinard commented Jan 12, 2018

ndabAP commented Jan 12, 2018

etpinard commented Jan 12, 2018

dy commented Nov 28, 2017 •

edited

Loading

dy Nov 28, 2017 •

edited

Loading

dy Nov 29, 2017 •

edited

Loading

dy commented Nov 28, 2017 •

edited

Loading

ndabAP commented Jan 12, 2018 •

edited

Loading